Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Proposal] Forbid usage of the MediaKeys type and other EME TS types #1477

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

[Proposal] Forbid usage of the MediaKeys type and other EME TS types #1477

wants to merge 2 commits into from
+244 −334

Conversation

peaBerberian
Copy link
Collaborator

@peaBerberian peaBerberian commented Jul 5, 2024 •
edited
Loading

Based on #1397.

In the idea of including fake encrypted contents in our integration tests by mocking MSE+EME+HTML 5 media API (not as complex as it sounds) to increase by a lot the types of issues our CI is capable to catch, I noticed an opportunity to even improve on the current code.

Like in #1397, the idea is there to provide typing subsets for the various EME API and to rely on them in the RxPlayer code (and enforcing this through our linter).

This allows to:

  • much simplify EME API mocking by not having to implement the full extent of the EME API (though what is missing in the custom types are very minor here: the EventTarget's third options optional parameter which we never use, dispatchEvent, and the onevent methods that we never rely on).

  • Allow the definition of environment-specific APIs - such as webkit-vendored API

  • Be more aware of which EME API we currently use

The gain is here much more evident than in #1397 as we already had some kind of subtyping with the ICustom... types (e.g. ICustomMediaKeys). By renaming those I... (e.g. IMediaKeys) and ensuring they are actually compatible with the base type (e.g. MediaKeys), we end up in my opinion with simpler code.

@peaBerberian peaBerberian added DRM Relative to DRM (EncryptedMediaExtensions) proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it labels Jul 5, 2024
@peaBerberian peaBerberian mentioned this pull request Jul 9, 2024
Open
@peaBerberian peaBerberian added this to the 4.2.0 milestone Jul 9, 2024
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from c4aae81 to a7cc348 Compare July 24, 2024 16:49
@peaBerberian peaBerberian added the Priority: 3 (Low) This issue or PR has a low priority. label Jul 26, 2024
@peaBerberian peaBerberian changed the base branch from misc/imediaelement to dev July 31, 2024 09:07
Florent-Bouisset
Florent-Bouisset reviewed Aug 1, 2024
View reviewed changes
src/compat/browser_compatibility_types.ts Outdated Show resolved Hide resolved
src/compat/eme/custom_media_keys/ie11_media_keys.ts Show resolved Hide resolved
src/compat/eme/custom_media_keys/old_webkit_media_keys.ts
Comment on lines 84 to +85
const onSessionRelatedEvent = (evt: Event) => {
this.trigger(evt.type, evt);
this.trigger(evt.type as keyof MediaKeySessionEventMap, evt);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can type onSessionRelatedEvent with MediaKeyMessageEvent | Event

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 84? What would that change?

Copy link
Collaborator

@Florent-Bouisset Florent-Bouisset Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean events like "keymessage", "message", "keyadded", "ready", "keyerror", "error" seems to be typed as a generic Event but it can be more precize with a type MediaKeyMessageEvent.

I don't really understand why typescript correctly show an error for

 this.trigger("message", evt);

but does not trigger an error for

this.trigger(evt.type as keyof MediaKeySessionEventMap, evt);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to fix this, if you have an idea, you can commit it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about how is it supposed to work on the "old webkit", from what I read, we can have an event
triggered from error with a type that is different, it seems weird ?

mediaElement.addEventListener("error", (event) => event.type /* keystatuseschange*/)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? How is it possible? Where did you see that possibility?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that event "type" was more precised in typescript but it's always a string
In fact what we can do is:

    const onSessionRelatedEvent = (evt: MediaKeyMessageEvent | Event) => {
      this.trigger(evt.type, evt);
    };

but I'm not sure if it's better than casting

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm kind of lost on that one.

In any case, the compat EME files are the "ugliest" TypeScript files in the RxPlayer to me but it makes sense that it is (due to the beauty of EME implementations out there).

@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from e210921 to 6429671 Compare August 1, 2024 13:53
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from 231f5b1 to d52f7b9 Compare August 13, 2024 16:50
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 3 times, most recently from 74fed2d to 29e63ac Compare September 4, 2024 17:53
@peaBerberian peaBerberian modified the milestones: 4.2.0, 4.3.0 Oct 4, 2024
@peaBerberian peaBerberian force-pushed the misc/no-eme-type branch 2 times, most recently from 6cb2c4b to 1cdf76b Compare December 16, 2024 17:54
@peaBerberian
[Proposal] Forbid usage of the MediaKeys type and other EME TS types
84cbf4e 
Based on #1397.

In the idea of including fake encrypted contents in our integration
tests by mocking MSE+EME+HTML 5 media API (not as complex as it sounds)
to increase by a lot the types of issues our CI is capable to catch, I
noticed an opportunity to even improve on the current code.

Like in #1397, the idea is there to provide typings subset of the
various EME API and to rely on them instead in the RxPlayer code (and
enforcing this through our linter).

This allows to:

  - much simplify EME API mocking by not having to implement the full
    extent of the EME API - though almost all of it is implemented
    instead (exceptions are the `EventTarget`'s third `options` optional
    parameter which we never use, `dispatchEvent`, and the `onevent`
    methods that we never rely on).

  - Allow the definition of environment-specific APIs

  - Be more aware of which EME API we currently use

The gain is here much more evident than in #1397 as we already had some
kind of sub-typings with the `ICustom...` types (e.g.
`ICustomMediaKeys`). By renaming those `I...` (e.g. `IMediaKeys`) and
ensuring they are actually compatible with the base type (e.g.
`MediaKeys`), we end up in my opinion with simpler code.
@peaBerberian
Rewrite type check compatibility trick after review
637ba4f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Florent-Bouisset Florent-Bouisset Florent-Bouisset left review comments

Assignees
No one assigned
Labels
DRM Relative to DRM (EncryptedMediaExtensions) Priority: 3 (Low) This issue or PR has a low priority. proposal This Pull Request or Issue is only a proposal for a change with the expectation of a debate on it
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@peaBerberian @Florent-Bouisset